Skip to content

feat(mro): Improve martian_make_mro #498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 12, 2024
Merged

feat(mro): Improve martian_make_mro #498

merged 4 commits into from
Jun 12, 2024

Conversation

sjackman
Copy link
Contributor

@sjackman sjackman commented Jun 12, 2024

  • Improve error message of martian_make_mro
  • Use current_executable as fallback in get_generator_name
  • Deref symlinks in current_executable

Slack: https://10xgenomics.slack.com/archives/CJT1RB554/p1717443208330939

Copy link
Member

@sreenathkrishnan sreenathkrishnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely better! Were you able to check that the generated mros did not change as a sanity check?

);
ensure!(
rewrite || !filename.exists(),
"File {} exists. Use --rewrite to overwrite it.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--rewrite is a caller specific detail, but this is valid for all our use cases

@sjackman sjackman force-pushed the sj/martian_make_mro branch from 3c5e88f to 6c63611 Compare June 12, 2024 17:00
@sjackman
Copy link
Contributor Author

Definitely better! Were you able to check that the generated mros did not change as a sanity check?

Use current_executable rather than get_generator_name in make_mro_string

This change did a cause change in the MRO files produced by the tests, though it was fine for the non-test code. I've reverted this change for now. I may take another look at it in a separate PR.

@sjackman sjackman merged commit 13e107e into master Jun 12, 2024
2 checks passed
@sjackman sjackman deleted the sj/martian_make_mro branch June 12, 2024 21:01
let args: Vec<_> = std::env::args().collect();
std::path::Path::new(&args[0])
Path::new(&std::env::args().next().unwrap())
.canonicalize()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this. Cannonicalization makes this function essentially useless in a bazel runfiles tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants